853209 - Rewrite CPOWs to use one actor per process

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

In js/ipc we have some preliminary code to implement cross-process object wrappers. The idea is that we can share JavaScript objects across process boundaries, by allowing a parent process to hold a wrapper object, and having that object's class ops send JSAPI queries over IPC.

The existing CPOW code uses per-wrapper actors, and these actors are managed by per-context actors. For efficiency, we'd like to use one actor per process, and have each actor keep track of its wrapped objects.

The existing code is not used, except for its own test cases and an unused xpcshell function, so the plan is to remove all of it. The new code still won't be used anywhere (except for our electrolysis experiments), but it'll use the desired actor model, retain most of the old ipc code's JSAPI logic, and add support for GC.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: basically the idea.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: remove existing CPOW implementation

The existing CPOWs aren't used, and won't quite work for e10s. To make review of the new CPOWs easier, I'm splitting out the removal into a separate patch.

(Bill, I ran into the same header issues so I just left a skeletal js/ipc folder.)

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 743858 [details] [diff] [review]
remove existing CPOW implementation

Review of attachment 743858 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: new CPOWs

Bill rejiggered much of the CPOW code a while back to use Proxies instead of a custom JSClass. Eventually, we'll probably have to move CPOWs into xpconnect to properly participate in the wrapping process, but what we have works for now.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 747202 [details] [diff] [review]
new CPOWs

Bobby, there are a few xpconnect changes in here it would be good to have your feedback on:

(1) Avoid double-wrapping CPOWs, for compat with new DOM bindings.
(2) Prevent passing CPOWs as an nsISupports to a C++ function, which is unsafe.
(3) Create a custom JSClass for XPCOutParams, so we can marshal them over IPC.
(4) Make "cpow instanceof Ci.whatevs" work.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 747202 [details] [diff] [review]
new CPOWs

Review of attachment 747202 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't look at the xpconnect changes. I'm assuming Bobby is responsible for those. You should probably have somebody else review the changes to dom/ too, since I'm not a peer there.

I ended up accumulating a lot of nits, but everything looks fine.

::: dom/ipc/ContentChild.cpp
@@ +563,5 @@
> +    JSRuntime *rt;
> +    svc->GetRuntime(&rt);
> +    NS_ENSURE_TRUE(svc, NULL);
> +
> +    mozilla::jsipc::JavaScriptChild *compartment = new mozilla::jsipc::JavaScriptChild(rt);

It doesn't really make sense to call this |compartment| any more.

@@ +574,5 @@
> +
> +bool
> +ContentChild::DeallocPJavaScript(PJavaScriptChild *compartment)
> +{
> +    mozilla::jsipc::JavaScriptChild* parent = static_cast<mozilla::jsipc::JavaScriptChild *>(compartment);

I don't think this is necessary since the PJavaScriptChild destructor is virtual, so the JavaScripChild dtor is too.

::: dom/ipc/ContentParent.cpp
@@ +1011,5 @@
>      }
>  }
>  
> +jsipc::JavaScriptParent*
> +ContentParent::GetJavaScript()

It looks like this needs to be updated with the changes to prevent races (rev be73b9e41990 on larch).

@@ +1595,5 @@
>  
> +mozilla::jsipc::PJavaScriptParent *
> +ContentParent::AllocPJavaScript()
> +{
> +    mozilla::jsipc::JavaScriptParent *compartment = new mozilla::jsipc::JavaScriptParent();

Should be named something else.

::: js/ipc/JavaScriptChild.cpp
@@ +48,5 @@
> +        return false;
> +    if (!ids_.init())
> +        return false;
> +
> +    JS_AddExtraGCRootsTracer(rt_, Trace, this);

This can fail.

@@ +55,5 @@
> +
> +bool
> +JavaScriptChild::RecvDropObject(const ObjectId &objId)
> +{
> +    JSObject *obj = objects_.find(objId);

Can we wrap this code in a method, here and elsewhere?

@@ +56,5 @@
> +bool
> +JavaScriptChild::RecvDropObject(const ObjectId &objId)
> +{
> +    JSObject *obj = objects_.find(objId);
> +    if (obj) {

Why would this ever be NULL?

@@ +64,5 @@
> +    return true;
> +}
> +
> +static inline bool
> +ToId(JSContext *cx, const nsString &from, jsid *to)

Maybe call this ConvertGeckoStringToId?

@@ +70,5 @@
> +    JSString *str = JS_NewUCStringCopyN(cx, from.BeginReading(), from.Length());
> +    if (!str)
> +        return false;
> +
> +    return JS_ValueToId(cx, STRING_TO_JSVAL(str), to);

StringValue

@@ +74,5 @@
> +    return JS_ValueToId(cx, STRING_TO_JSVAL(str), to);
> +}
> +
> +ObjectId
> +JavaScriptChild::Send(JSContext *cx, JSObject *obj)

Is this used by anything? It's oddly named, since it doesn't send anything.

@@ +99,5 @@
> +        return true;
> +    }
> +
> +    id = ++lastId_;
> +    if (id > (unsigned(JSVAL_INT_MAX) >> OBJECT_EXTRA_BITS))

Hmm, this is actually kind of bad. Can we make ObjectId be a uint64_t? That way we're much less likely to run out of them, since they're never recycled.

@@ +119,5 @@
> +JSObject *
> +JavaScriptChild::unwrap(JSContext *cx, ObjectId id)
> +{
> +    JSObject *obj = objects_.find(id);
> +    if (!obj) {

Can this be made an assertion? It seems like it would only happen if there's a bug in our code.

@@ +303,5 @@
> +    RootedValue objv(cx);
> +    if (!toValue(cx, argv[0], &objv))
> +        return fail(cx, rs);
> +
> +    JSAutoCompartment comp(cx, JSVAL_TO_OBJECT(objv));

.toObject()

@@ +313,5 @@
> +    for (size_t i = 0; i < argv.Length(); i++) {
> +        if (argv[i].type() == JSParam::Tvoid_t) {
> +            // This is an outparam.
> +            JSCompartment *compartment = js::GetContextCompartment(cx);
> +            JSObject *global = JS_GetGlobalForCompartmentOrNull(cx, compartment); 

Trailing space.

@@ +317,5 @@
> +            JSObject *global = JS_GetGlobalForCompartmentOrNull(cx, compartment); 
> +            JSObject *obj = xpc_NewOutObject(cx, global);
> +            if (!obj)
> +                return fail(cx, rs);
> +            if (!outobjects.append(OBJECT_TO_JSVAL(obj)))

ObjectValue

@@ +319,5 @@
> +            if (!obj)
> +                return fail(cx, rs);
> +            if (!outobjects.append(OBJECT_TO_JSVAL(obj)))
> +                return fail(cx, rs);
> +            if (!vals.append(OBJECT_TO_JSVAL(obj)))

ObjectValue

@@ +353,5 @@
> +    // field, and add it to a temporary list. We need to do this separately
> +    // because the outparams vector is not rooted.
> +    vals.clear();
> +    for (size_t i = 0; i < outobjects.length(); i++) {
> +        JSObject *obj = JSVAL_TO_OBJECT(outobjects[i]);

.toObject

@@ +361,5 @@
> +        if (JS_HasProperty(cx, obj, "value", &found)) {
> +            if (!JS_GetProperty(cx, obj, "value", &v))
> +                return fail(cx, rs);
> +        } else {
> +            v = JSVAL_VOID;

UndefinedValue

@@ +362,5 @@
> +            if (!JS_GetProperty(cx, obj, "value", &v))
> +                return fail(cx, rs);
> +        } else {
> +            v = JSVAL_VOID;
> +            outparams->ReplaceElementAt(i, JSParam(void_t()));

Is this line needed? It seems like everything would work fine without it.

@@ +371,5 @@
> +
> +    // Copy the outparams. If any outparam is already set to a void_t, we
> +    // treat this as the outparam never having been set.
> +    for (size_t i = 0; i < vals.length(); i++) {
> +        if (outparams->ElementAt(i).type() == JSParam::Tvoid_t)

Can we skip this case too?

::: js/ipc/JavaScriptChild.h
@@ +3,5 @@
> + *
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifndef mozilla_jsipc_JavaScriptWrapperChild_h_

Please update the name and put an empty line after the license.

::: js/ipc/JavaScriptParent.cpp
@@ +25,5 @@
> +
> +static inline JavaScriptParent *
> +ParentOf(JSObject *obj)
> +{
> +    return reinterpret_cast<JavaScriptParent *>(GetProxyExtra(obj, 0).toPrivate());

Can we assert that obj is a CPOWProxyHandler?

@@ +31,5 @@
> +
> +ObjectId
> +JavaScriptParent::IdOf(JSObject *obj)
> +{
> +    Value v = GetProxyExtra(obj, 1);

Same assert here too.

@@ +32,5 @@
> +ObjectId
> +JavaScriptParent::IdOf(JSObject *obj)
> +{
> +    Value v = GetProxyExtra(obj, 1);
> +    if (!v.isInt32())

Will this ever be the case? It seems better to assert here.

@@ +41,5 @@
> +    return objId;
> +}
> +
> +static bool
> +ToGecko(JSContext *cx, jsid id, nsString *to)

Maybe call it ConvertIdToGeckoString?

@@ +105,5 @@
> +
> +CPOWProxyHandler CPOWProxyHandler::singleton;
> +
> +bool
> +CPOWProxyHandler::getPropertyDescriptor(JSContext *cx, HandleObject proxy, HandleId id,

Could you re-order the methods in the file so that the ordering is consistent? It seems reasonable to define the CPOWProxyHandler:: method, and then the same JavaScriptParent:: method after it. The ordering of the methods should follow how they're laid out in the ProxyHandler definition.

@@ +167,5 @@
> +bool
> +JavaScriptParent::isExtensible(JSObject *proxy)
> +{
> +    uint32_t objId = IdOf(proxy);
> +    MOZ_ASSERT(objId);

Maybe we should have something like idOfNonNull that would assert this? It looks like there would be a fair number of users.

@@ +170,5 @@
> +    uint32_t objId = IdOf(proxy);
> +    MOZ_ASSERT(objId);
> +
> +    bool extensible;
> +    ReturnStatus status;

I think this var is unused.

@@ +191,5 @@
> +    MOZ_ASSERT(objId);
> +
> +    nsString idstr;
> +    if (!ToGecko(cx, id, &idstr))
> +        return JS_FALSE;

Can you do a search and replace to change JS_FALSE to false?

@@ +319,5 @@
> +    RootedValue v(cx);
> +    for (size_t i = 0; i < args.length() + 2; i++) {
> +        v = args.base()[i];
> +        if (v.isObject()) {
> +            JSObject *obj = JSVAL_TO_OBJECT(v);

Use .toObject()?

@@ +354,5 @@
> +        // object.
> +        if (!toValue(cx, outparams[i], &v))
> +            return false;
> +
> +        JSObject *obj = JSVAL_TO_OBJECT(outobjects[i]);

.toObject()

@@ +383,5 @@
> +{
> +    uint32_t objId = IdOf(proxy);
> +    MOZ_ASSERT(objId);
> +
> +    // This function is assumed infallible, so we just return false of the IPC

of -> if

@@ +429,5 @@
> +        return obj;
> +    }
> +
> +    bool callable = !!(objId & OBJECT_IS_CALLABLE);
> +    RootedObject someObj(cx, JS_GetGlobalForCompartmentOrNull(cx, GetContextCompartment(cx)));

It looks like we can just pass NULL for the parent. That seems like a better option.

@@ +430,5 @@
> +    }
> +
> +    bool callable = !!(objId & OBJECT_IS_CALLABLE);
> +    RootedObject someObj(cx, JS_GetGlobalForCompartmentOrNull(cx, GetContextCompartment(cx)));
> +    RootedObject someObj2(cx);

This doesn't seem to be used.

@@ +453,5 @@
> +    return obj;
> +}
> +
> +void
> +JavaScriptParent::drop(JSObject *obj)

Could you move this to be near the finalize hook?

@@ +459,5 @@
> +    if (inactive_)
> +        return;
> +
> +    uint32_t objId = IdOf(obj);
> +    if (!objId)

Why would this ever happen?

::: js/ipc/JavaScriptParent.h
@@ +32,5 @@
> +    bool set(JSContext *cx, JS::HandleObject proxy, JS::HandleObject receiver,
> +             JS::HandleId id, bool strict, JS::MutableHandleValue vp);
> +    bool call(JSContext *cx, JS::HandleObject proxy, const JS::CallArgs &args);
> +
> +    JSObject *Unwrap(JSContext *cx, ObjectId objId) {

Is this used?

@@ +39,5 @@
> +    }
> +
> +    void DecRef();
> +    void IncRef();
> +    void DestroyFromContent();

Can these method names be lowercase?

@@ +51,5 @@
> +    bool getPropertyDescriptor(JSContext *cx, JS::HandleObject proxy, JS::HandleId id,
> +                               JSPropertyDescriptor *desc, unsigned flags);
> +    bool objectClassIs(JSContext *cx, JS::HandleObject obj, js::ESClassValue classValue);
> +    bool preventExtensions(JSContext *cx, JS::HandleObject proxy);
> +    bool isExtensible(JSObject *proxy);

Can you move the declarations of these hooks so they go right after |call|?

@@ +58,5 @@
> +    JSObject *unwrap(JSContext *cx, ObjectId objId);
> +
> +  private:
> +    bool makeId(JSContext *cx, JSObject *obj, ObjectId *idp);
> +    ObjectId IdOf(JSObject *obj);

Can you call it |idOf|?

@@ +61,5 @@
> +    bool makeId(JSContext *cx, JSObject *obj, ObjectId *idp);
> +    ObjectId IdOf(JSObject *obj);
> +
> +    // Catastrophic IPC failure.
> +    JSBool ipcfail(JSContext *cx);

This should probably be ipcFail. Also, it could return just a bool.

@@ +67,5 @@
> +    // Check whether a return status is okay, and if not, propagate its error.
> +    bool ok(JSContext *cx, const ReturnStatus &status);
> +
> +  private:
> +    uint64_t refcount_;

This could just be uintptr_t.

::: js/ipc/JavaScriptShared.cpp
@@ +98,5 @@
> +
> +bool
> +JavaScriptShared::init()
> +{
> +    if (!objects_.init())

Just return the result?

@@ +111,5 @@
> +      case JSTYPE_VOID:
> +        *to = void_t();
> +        return true;
> +
> +      case JSTYPE_NULL:

Ugh. Apparently this case never happens and we get JSTYPE_NULL for null.

@@ +120,5 @@
> +
> +      case JSTYPE_OBJECT:
> +      case JSTYPE_FUNCTION:
> +      {
> +        JSObject *obj = JSVAL_TO_OBJECT(from);

Use .toObjectOrNull() ?

@@ +153,5 @@
> +      }
> +
> +      case JSTYPE_NUMBER:
> +        if (JSVAL_IS_INT(from))
> +            *to = double(JSVAL_TO_INT(from));

Would be nice to use .toInt32 and .toDouble.

@@ +159,5 @@
> +            *to = JSVAL_TO_DOUBLE(from);
> +        return true;
> +
> +      case JSTYPE_BOOLEAN:
> +        *to = !!JSVAL_TO_BOOLEAN(from);

...and here.

@@ +173,5 @@
> +JavaScriptShared::toValue(JSContext *cx, const JSVariant &from, MutableHandleValue to)
> +{
> +    switch (from.type()) {
> +        case JSVariant::Tvoid_t:
> +          to.set(JSVAL_VOID);

Can you use UndefinedValue() and such here and below?

@@ +272,5 @@
> +    out->shortid() = desc.shortid;
> +    if (!toVariant(cx, desc.value, &out->value()))
> +        return false;
> +
> +    // rooting.

What does this comment mean?

::: js/ipc/JavaScriptShared.h
@@ +4,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_jsipc_ContextShared_h__

Please update the name.

@@ +11,5 @@
> +#include "jsapi.h"
> +#include "jspubtd.h"
> +#include "js/HashTable.h"
> +#include "mozilla/jsipc/PJavaScript.h"
> +#include "nsJSUtils.h"

What's this for? Maybe it belongs in JavaScriptShared.cpp?

@@ +21,5 @@
> +
> +// Map ids -> JSObjects
> +class ObjectStore
> +{
> +    struct TableKeyHasher {

I think DefaultHasher<ObjectId> would work fine here.

@@ +80,5 @@
> +  protected:
> +    bool toVariant(JSContext *cx, jsval from, JSVariant *to);
> +    bool toValue(JSContext *cx, const JSVariant &from, JS::MutableHandleValue to);
> +    bool fromDesc(JSContext *cx, const JSPropertyDescriptor &desc, PPropertyDescriptor *out);
> +    bool toDesc(JSContext *cx, const PPropertyDescriptor &in, JSPropertyDescriptor *out);

Can you spell out Descriptor fully in the method names?

::: js/ipc/PJavaScript.ipdl
@@ +5,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +include protocol PContent;
> +include DOMTypes;

Is this for nsString? If not, do we need it?

@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace jsipc {
> +
> +struct JSIID {

Could you put the opening brace on its own line for consistency (there's a mixture in this file)?

@@ +29,5 @@
> +
> +union JSVariant {
> +    void_t;     /* |undefined| */
> +    nsString;   /* StringValue(x) */
> +    uint32_t;   /* JSObject */

This one might need a longer comment.

@@ +45,5 @@
> +    void_t;     /* value is strictly an xpc out param */
> +    JSVariant;  /* actual value to pass through */
> +};
> +
> +struct PPropertyDescriptor

The prefix here (and on JSParam) seems kind of unnecessary. Would it be possible to skip the prefixes and rely on the jsipc namespace to distinguish them?

@@ +72,5 @@
> +child:
> +    // The parent process no longer holds any references to the child object.
> +    async DropObject(uint32_t objId);
> +
> +    // JSAPI call wrappers.

Maybe say that these roughly parallel the ProxyHandler hooks.

@@ +83,5 @@
> +    urgent InstanceOf(uint32_t objId, JSIID iid) returns (ReturnStatus rs, bool instanceof);
> +    urgent GetPropertyDescriptor(uint32_t objId, nsString id, uint32_t flags) returns (ReturnStatus rs, PPropertyDescriptor result);
> +    urgent ObjectClassIs(uint32_t objId, uint32_t classValue) returns (bool result);
> +    urgent IsExtensible(uint32_t objId) returns (bool result);
> +    urgent PreventExtensions(uint32_t objId) returns (ReturnStatus rs);

Hmm, the first set of hooks all end in Hook while these don't. It would be good to standardize. I guess the Hook part probably isn't needed.

::: js/ipc/moz.build
@@ -1,1 @@
> -# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-

Why this change? Maybe a merge problem?

::: js/src/jsapi.h
@@ +2388,2 @@
>  extern JS_PUBLIC_API(void)
> +JS_RemoveExtraGCRootsTracer(JSRuntime *rt, JSTraceDataOp traceOp, void *data);

The comment for this isn't really right anymore. Maybe just remove it.

::: js/src/jscntxt.h
@@ +1145,5 @@
> +        void *data;
> +
> +        ExtraTracer()
> +          : op(NULL), data(NULL)
> +        { }

Most of the time we don't put a space between the braces. Also, this could probably all fit on one line.

Comment 7

[Andrew McCreight [:mccr8]]

smaug might be a good reviewer for the DOM stuff.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 747202 [details] [diff] [review]
new CPOWs

Yeah, smaug should review the dom IPC stuff. I'm almost finished reviewing the XPConnect changes.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 747202 [details] [diff] [review]
new CPOWs

Review of attachment 747202 [details] [diff] [review]:
-----------------------------------------------------------------

XPConnect stuff looks on the right track, but there's enough to be done that I'd like to look at it again.

::: js/xpconnect/src/Makefile.in
@@ +69,3 @@
>  		$(NULL)
>  
> +include $(topsrcdir)/ipc/chromium/chromium-config.mk

What's this for?

::: js/xpconnect/src/XPCConvert.cpp
@@ +333,5 @@
>  
>                          return XPCVariant::VariantDataToJS(lccx, variant,
>                                                             pErr, d);
>                      }
> +                    // Don't double wrap CPOWs. This is a temporary measure

Shouldn't this all go in NativeInterface2JSObject? It's not clear to me why it's here and not there, except perhaps the fact that NativeInterface2JSObject is a hotter function?

Anyway, I think this should be ok to put there, just after all the wrappercache stuff.

@@ +341,5 @@
> +                    // the appropriate wrappers in place.
> +                    if (JSObject *cpow = UnwrapNativeCPOW(iface)) {
> +                        XPCCallContext &ccx = lccx.GetXPCCallContext();
> +                        if (!ccx.IsValid())
> +                            return false;

Just use lccx.GetJSContext() here - no need to force an entire ccx to spin up.

::: js/xpconnect/src/XPCJSID.cpp
@@ +480,5 @@
> +    RootedObject obj(cx, objArg);
> +
> +    RootedObject unwrapped(cx, js::CheckedUnwrap(obj, /* stopAtOuter = */ false));
> +    if (unwrapped && mozilla::jsipc::JavaScriptParent::IsCPOW(unwrapped))
> +        return unwrapped;

Why is this necessary?

@@ +529,5 @@
> +        return NS_OK;
> +    }
> +
> +    if (mozilla::jsipc::JavaScriptParent::IsCPOW(obj))
> +        return mozilla::jsipc::JavaScriptParent::InstanceOf(obj, iid, bp);

This was the only actual change in the body of this function, right? Just want to make sure, given that I only skimmed the rest.

@@ +576,2 @@
>  
> +    // is this really a native xpcom object with a wrapper?

This comment doesn't really make sense anymore.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2701,5 @@
>      JS_SetGCCallback(mJSRuntime, GCCallback);
>      mPrevGCSliceCallback = JS::SetGCSliceCallback(mJSRuntime, GCSliceCallback);
>      JS_SetFinalizeCallback(mJSRuntime, FinalizeCallback);
> +    if (!JS_AddExtraGCRootsTracer(mJSRuntime, TraceBlackJS, this))
> +      NS_RUNTIMEABORT("JS_AddExtraGCRootsTracer failed.");

Why do we want this?

::: js/xpconnect/src/XPCWrappedJSClass.cpp
@@ +1353,5 @@
> +            RootedObject out_obj(cx);
> +            if  (param.IsIn())
> +                out_obj = NewInOutObject(cx, obj);
> +            else
> +                out_obj = xpc_NewOutObject(cx, obj);

Hm, why the distinction between in and inout here? Shouldn't we be able to marshal both?

Also, these two functions should have the same linkage/namespacing etc. If neither needs to be externally accessible, just make them static and drop the prefix. Otherwise, put them in namespace xpc.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +2124,5 @@
>  
>      jsval* const mArgv;
>      const uint32_t mArgc;
>  
> +    bool mIsNativeMethod;

how about mIsDoubleWrappedCall (inverted, of course)?

@@ +2185,5 @@
>          , mJSContextIndex(UINT8_MAX)
>          , mOptArgcIndex(UINT8_MAX)
>          , mArgv(ccx.GetArgv())
>          , mArgc(ccx.GetArgc())
> +        , mIsNativeMethod(true)

Any reason not to just initialize this with IsWrappedJSClass(mCallee) right here?

@@ +2721,5 @@
>          ThrowBadParam(err, i, mCallContext);
>          return false;
>      }
>  
> +    if (mIsNativeMethod && src.isObject() &&

This needs a very big comment explaining why we have to handle this in CallMethod and why we can't just forbid creating an XPCWrappedJS for CPOWs (which, conceptually, is what we should be doing).

@@ +2736,5 @@
> +                return false;
> +            }
> +        }
> +    }
> +

Wait, why do we need to do all this junk? Why can't we just check whether src is a CPOW?

This function was very hot in the old world. Probably much less so now, but we shouldn't add more stuff to it than we need to.

::: js/xpconnect/src/xpcprivate.h
@@ +3106,5 @@
>      uint32_t* mDescriptors;
>  };
>  
> +JSObject *xpc_NewOutObject(JSContext* cx, JSObject* scope);
> +bool xpc_IsOutObject(JSContext* cx, JSObject* obj);

xpc_ prefixing is deprecated. Put stuff in namespace xpc instead.

@@ +3713,5 @@
>  extern JSBool
>  xpc_JSObjectIsID(JSContext *cx, JSObject* obj);
>  
> +extern nsresult
> +xpc_HasInstance(JSContext *cx, JS::HandleObject obj, const nsID *iid, bool *bp);

same here.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 747202 [details] [diff] [review]
new CPOWs


>+mozilla::jsipc::PJavaScriptChild *
extra space before *

>+ContentChild::AllocPJavaScript()
>+{
>+    nsCOMPtr<nsIJSRuntimeService> svc = do_GetService("@mozilla.org/js/xpc/RuntimeService;1");
>+    NS_ENSURE_TRUE(svc, NULL);
>+
>+    JSRuntime *rt;
>+    svc->GetRuntime(&rt);
>+    NS_ENSURE_TRUE(svc, NULL);
>+
>+    mozilla::jsipc::JavaScriptChild *compartment = new mozilla::jsipc::JavaScriptChild(rt);
>+    if (!compartment->init()) {
>+        delete compartment;
>+        return NULL;
s/NULL/nullptr/

>+bool
>+ContentChild::DeallocPJavaScript(PJavaScriptChild *compartment)
I know the file uses some odd and inconsistent coding style, but
Foo* aParamName



>+jsipc::JavaScriptChild *
>+ContentChild::GetJavaScript()
GetJavaScript sounds very generic name to me.
Does it return some thing which is the current JavaScript code in content process or some runtime or what.
Would something like GetCPOWManager() or some such work?


>@@ -55,16 +55,17 @@ LOCAL_INCLUDES += \
> 	-I$(topsrcdir)/toolkit/xre \
> 	-I$(topsrcdir)/hal/sandbox \
> 	-I$(topsrcdir)/dom/mobilemessage/src/ipc \
> 	-I$(topsrcdir)/dom/devicestorage \
> 	-I$(topsrcdir)/widget/xpwidgets \
> 	-I$(topsrcdir)/dom/bluetooth \
> 	-I$(topsrcdir)/dom/bluetooth/ipc \
> 	-I$(topsrcdir)/content/media/webspeech/synth/ipc \
>+        -I$(topsrcdir)/js/ipc \
Use tabs as the rest of the -Is

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Bill McCloskey (:billm) from comment #6)
> Comment on attachment 747202 [details] [diff] [review]
> new CPOWs

Okay I applied most of these nits. Some answers/comments below:

> 
> @@ +99,5 @@
> > +        return true;
> > +    }
> > +
> > +    id = ++lastId_;
> > +    if (id > (unsigned(JSVAL_INT_MAX) >> OBJECT_EXTRA_BITS))
> 
> Hmm, this is actually kind of bad. Can we make ObjectId be a uint64_t? That
> way we're much less likely to run out of them, since they're never recycled.

I originally tried that, but then I realized jsvals couldn't represent 64-bit integers. I guess we could fake it, with a private ptr or something.

> @@ +362,5 @@
> > +            if (!JS_GetProperty(cx, obj, "value", &v))
> > +                return fail(cx, rs);
> > +        } else {
> > +            v = JSVAL_VOID;
> > +            outparams->ReplaceElementAt(i, JSParam(void_t()));
> 
> Is this line needed? It seems like everything would work fine without it.

It's used to signal that the outparam was never set. But I guess it doesn't matter. The difference is that now the parent will receive back "{ value: undefined }" instead of "{}".

> 
> @@ +167,5 @@
> > +bool
> > +JavaScriptParent::isExtensible(JSObject *proxy)
> > +{
> > +    uint32_t objId = IdOf(proxy);
> > +    MOZ_ASSERT(objId);
> 
> Maybe we should have something like idOfNonNull that would assert this? It
> looks like there would be a fair number of users.

Looks like this would apply to all the cases so I moved the assert into IdOf().

> @@ +429,5 @@
> > +        return obj;
> > +    }
> > +
> > +    bool callable = !!(objId & OBJECT_IS_CALLABLE);
> > +    RootedObject someObj(cx, JS_GetGlobalForCompartmentOrNull(cx, GetContextCompartment(cx)));
> 
> It looks like we can just pass NULL for the parent. That seems like a better
> option.

Okay.
 
> @@ +61,5 @@
> > +    bool makeId(JSContext *cx, JSObject *obj, ObjectId *idp);
> > +    ObjectId IdOf(JSObject *obj);
> > +
> > +    // Catastrophic IPC failure.
> > +    JSBool ipcfail(JSContext *cx);
> 
> This should probably be ipcFail. Also, it could return just a bool. 

I like ipcfail() better, since its a common error thing.

> ::: js/ipc/JavaScriptShared.cpp
> @@ +98,5 @@
> > +
> > +bool
> > +JavaScriptShared::init()
> > +{
> > +    if (!objects_.init())
> 
> Just return the result?

I usually don't do that for init() in case I have to add things later.

> 
> @@ +111,5 @@
> > +      case JSTYPE_VOID:
> > +        *to = void_t();
> > +        return true;
> > +
> > +      case JSTYPE_NULL:
> 
> Ugh. Apparently this case never happens and we get JSTYPE_NULL for null.

Err, you mean, JSTYPE_OBJECT, with a NULL pointer right? Yeah, that one confused me too :)

> @@ +272,5 @@
> > +    out->shortid() = desc.shortid;
> > +    if (!toVariant(cx, desc.value, &out->value()))
> > +        return false;
> > +
> > +    // rooting.
> 
> What does this comment mean?

I'm not sure. I took it out. Maybe prop descs aren't rooted?

> The prefix here (and on JSParam) seems kind of unnecessary. Would it be
> possible to skip the prefixes and rely on the jsipc namespace to distinguish
> them?

I like having a separate name better but I don't have a strong argument for why.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: remove old CPOWs

rebased, carrying over R+

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: new CPOWs, v2

new CPOWs rebased and with bill's requested changes applied, except for the uint32_t overflow thing which I haven't thought enough about yet.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Bobby Holley (:bholley) from comment #9)
> ::: js/xpconnect/src/Makefile.in
> @@ +69,3 @@
> >  		$(NULL)
> >  
> > +include $(topsrcdir)/ipc/chromium/chromium-config.mk
> 
> What's this for?

It's needed to include ipc stuff.

> @@ +529,5 @@
> > +        return NS_OK;
> > +    }
> > +
> > +    if (mozilla::jsipc::JavaScriptParent::IsCPOW(obj))
> > +        return mozilla::jsipc::JavaScriptParent::InstanceOf(obj, iid, bp);
> 
> This was the only actual change in the body of this function, right? Just
> want to make sure, given that I only skimmed the rest.

Yeah.

> ::: js/xpconnect/src/XPCWrappedJSClass.cpp
> @@ +1353,5 @@
> > +            RootedObject out_obj(cx);
> > +            if  (param.IsIn())
> > +                out_obj = NewInOutObject(cx, obj);
> > +            else
> > +                out_obj = xpc_NewOutObject(cx, obj);
> 
> Hm, why the distinction between in and inout here? Shouldn't we be able to
> marshal both?

It's more about what's being marshalled behind the object, and I haven't hit any cases that need inout yet, so I'm not sure. This was a special hack for GetInterfaces() which only uses out objects to pass an integer. Primitives are okay. Anyway, I can do it if you'd like.

Comment 15

[Bobby Holley (:bholley)]

(In reply to David Anderson [:dvander] from comment #14)
> It's more about what's being marshalled behind the object, and I haven't hit
> any cases that need inout yet, so I'm not sure. This was a special hack for
> GetInterfaces() which only uses out objects to pass an integer. Primitives
> are okay. Anyway, I can do it if you'd like.

Please just use the same object type for both cases, and assert against marshaling that object in the other direction if you like.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Bobby Holley (:bholley) from comment #9)
> Comment on attachment 747202 [details] [diff] [review]
> new CPOWs
> 
> Review of attachment 747202 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +2736,5 @@
> > +                return false;
> > +            }
> > +        }
> > +    }
> > +
> 
> Wait, why do we need to do all this junk? Why can't we just check whether
> src is a CPOW?
> 
> This function was very hot in the old world. Probably much less so now, but
> we shouldn't add more stuff to it than we need to.

Bill says it used to work this way, but we also have to check whether the src is a double-wrapped CPOW, so it was easier to check if the result is a double-wrapped CPOW. see: https://hg.mozilla.org/projects/larch/diff/ebdb21a85740/js/xpconnect/src/XPCWrappedNative.cpp

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: new cpows, v3

for xpconnect changes

(btw, it was fun rebasing this against the CallContext/slimwrapper removal - so much nicer!)

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: use 64-bit ids

follow-up patch to use 47-bit IDs to avoid 32-bit overflow

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 765740 [details] [diff] [review]
use 64-bit ids

Review of attachment 765740 [details] [diff] [review]:
-----------------------------------------------------------------

It seems like we could get 62 bits out of this thing, but 47 is probably more than enough :-).

::: js/ipc/JavaScriptParent.cpp
@@ +533,5 @@
>              return NULL;
>          return obj;
>      }
>  
> +    if (objId > MAX_CPOW_IDS) {

It seems like this could be a MOZ_ASSERT since we never generate such large IDs.

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

I figured a hijacked child process might be able to send a bogus id or something.

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Oh yeah, good point.

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 765739 [details] [diff] [review]
new cpows, v3

Review of attachment 765739 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +2055,5 @@
>          , mJSContextIndex(UINT8_MAX)
>          , mOptArgcIndex(UINT8_MAX)
>          , mArgv(ccx.GetArgv())
>          , mArgc(ccx.GetArgc())
> +        , mIsDoubleWrappedCall(!nsXPCWrappedJSClass::IsWrappedJS(mCallee))

This name is backwards, along with the check below.

@@ +2602,5 @@
> +            if (mozilla::jsipc::JavaScriptParent::IsCPOW(wrappedobj)) {
> +                ThrowBadParam(NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE, i, mCallContext);
> +                return false;
> +            }
> +        }

Per IRC discussion, I don't think comment 16 applies anymore.

Comment 23

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

Comment 24

[Bobby Holley (:bholley)]

Comment on attachment 768041 [details] [diff] [review]
patch

Review of attachment 768041 [details] [diff] [review]:
-----------------------------------------------------------------

r- on account of the builtinclass thing, but I'll flip it if I'm mistaken there for some reason (which I may well be).

::: js/xpconnect/src/XPCConvert.cpp
@@ +854,5 @@
> +    // Don't double wrap CPOWs. This is a temporary measure for compatibility
> +    // with objects that don't provide necessary QIs (such as objects under
> +    // the new DOM bindings). We expect the other side of the CPOW to have
> +    // the appropriate wrappers in place.
> +    if (JSObject *cpow = UnwrapNativeCPOW(aHelper.GetCanonical())) {

Hm, do we need the canonical isupports here? UnwrapNativeCPOW just QIs the pointer, so the regular one should be fine I'd think, and potentially faster.

::: js/xpconnect/src/XPCWrappedJSClass.cpp
@@ +1652,5 @@
>  
> +static void
> +FinalizeStub(JSFreeOp *fop, JSObject *obj)
> +{
> +}

What's this about?

@@ +1673,5 @@
> +    NULL    /* trace */
> +};
> +
> +bool
> +xpc::IsInOutObject(JSContext* cx, JSObject* obj)

I think I'd prefer to call these OutObjects, even if we also use them for |inout|, because the out-ness is what makes them special.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +2585,5 @@
> +        mozilla::jsipc::JavaScriptParent::IsCPOW(wrappedobj))
> +    {
> +        ThrowBadParam(NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE, i, mCallContext);
> +        return false;
> +    }

This should probably move above the call to JSData2Native.

Also, it seems to me XPCWrappedJS::GetNewOrUsed is still going to bork when you pass a CPOW to JSData2Native, right? It seems like you need some sort of back-door to indicate that implementing the builtinclass is ok.

Comment 25

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: new cpows, v4

Rolled up CPOW patch, carrying over review. XPCWrappedNative changes will come separately.

Comment 26

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c45375d507e

try run: https://tbpl.mozilla.org/?tree=Try&rev=411857d1fb6f

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0c45375d507e

Comment 28

[Bobby Holley (:bholley)]

I ended up having to backport this:

https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=3266c1d73816

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Bustage fix for Windows.
https://hg.mozilla.org/releases/mozilla-aurora/rev/cd307124f11f

Comment 30

[u279076]

Assuming this doesn't need QA.